New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: fix test-process-kill-null.js for Windows #14099

Closed

starkwang wants to merge 1 commit into nodejs:master from starkwang:fix-test-process-kill-null

Contributor

starkwang commented Jul 6, 2017

The test-process-kill-null.js failed in Windows because cat command is invalid for Windows cmd.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

nodejs-github-bot added the test label

Member

richardlau commented Jul 6, 2017

The test-process-kill-null.js failed in Windows because cat command is invalid for Windows cmd.

It's included in Git bash. From BUILDING.md:

Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH.

starkwang closed this

refack reopened this

Contributor

refack commented Jul 7, 2017

I want this.
The MSYS cat is a ported utility and it's not as robust on windows as it is on *nix.
If we could remove the git-for-windows dependancy, all the better.

refack suggested changes

View reviewed changes

test/parallel/test-process-kill-null.js Outdated

    
            @@ -24,24 +24,24 @@ require('../common');
          
              const assert = require('assert');

              const spawn = require('child_process').spawn;

              const cat = spawn('cat');

              const child = process.platform === 'win32' ? spawn('cmd.exe') : spawn('cat');

Contributor

refack Jul 7, 2017

The recommended test is common.isWindows (assign const common on L23)

test/parallel/test-process-kill-null.js Outdated

    
              cat.on('exit', function() {

              child.on('exit', function() {

Contributor

refack Jul 7, 2017

add common.mustCall

test/parallel/test-process-kill-null.js Outdated

    
            @@ -24,24 +24,24 @@ require('../common');
          
              const assert = require('assert');

              const spawn = require('child_process').spawn;

              const cat = spawn('cat');

              const child = process.platform === 'win32' ? spawn('cmd.exe') : spawn('cat');

              let called;

Contributor

refack Jul 7, 2017

not needed

test/parallel/test-process-kill-null.js Outdated

    
                }, Error);

              });

              cat.stdout.on('data', function() {

              child.stdout.on('data', function() {

Contributor

refack Jul 7, 2017

add common.mustCall

test/parallel/test-process-kill-null.js Outdated

    
                }, Error);

              });

              cat.stdout.on('data', function() {

              child.stdout.on('data', function() {

                called = true;

Contributor

refack Jul 7, 2017

not needed

test/parallel/test-process-kill-null.js Outdated

    
              });

              // EPIPE when null sig fails

              cat.stdin.write('test');

              child.stdin.write('test');

              process.on('exit', function() {

Contributor

refack Jul 7, 2017

Not needed.

refack added process windows labels


          test: fix test-process-kill-null.js for Windows

310a19c

starkwang force-pushed the fix-test-process-kill-null branch from 7b96888 to 310a19c Compare

July 7, 2017 02:25

Contributor Author

starkwang commented Jul 7, 2017

Pushed commit to address comments.

refack approved these changes

View reviewed changes

Contributor

refack commented Jul 7, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10998/

refack pushed a commit to refack/node that referenced this pull request


          test: improve test-process-kill-null for Windows

44483b6

PR-URL: nodejs#14099
Reviewed-By: Refael Ackermann <refack@gmail.com>

Contributor

refack commented Jul 8, 2017

landed in 44483b6

refack closed this

Contributor

refack commented Jul 8, 2017 •

edited

Loading

Sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/7155/ ✔️
Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10276/

Contributor

refack commented Jul 8, 2017 •

edited

Loading

@starkwang ~~might~~ will need a followup PR to fix this flakiness:

not ok 235 parallel/test-process-kill-null
  ---
  duration_ms: 0.255
  severity: fail
  stack: |-
    internal/process.js:190
          throw errnoException(err, 'kill');
          ^
    
    Error: kill ESRCH
        at exports._errnoException (util.js:1023:11)
        at process.kill (internal/process.js:190:13)
        at Socket.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2012r2\test\parallel\test-process-kill-null.js:38:11)
        at Socket.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2015\label\win2012r2\test\common\index.js:518:15)
        at emitOne (events.js:115:13)
        at Socket.emit (events.js:210:7)
        at addChunk (_stream_readable.js:252:12)
        at readableAddChunk (_stream_readable.js:239:11)
        at Socket.Readable.push (_stream_readable.js:197:10)
        at Pipe.onread (net.js:589:20)
  ...

https://ci.nodejs.org/job/node-test-binary-windows/9711/RUN_SUBSET=3,VS_VERSION=vs2015,label=win2012r2/

Running another windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/10278/

refack mentioned this pull request

Temp revert 44483b6 #14142

Merged

3 tasks

refack added a commit to refack/node that referenced this pull request


          Revert "test: improve test-process-kill-null for Windows"

d69ecc6

This reverts commit 44483b6.

PR-URL: nodejs#14142
Fixes: nodejs#14141
Refs: nodejs#14099
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

refack reopened this

Contributor

refack commented Jul 9, 2017

@nodejs/platform-windows could the kill ESRCH ("No such process") actually be a bug?
micro context:

child.stdout.on('data', common.mustCall(function() {
  process.kill(child.pid, 'SIGKILL');
}

Member

richardlau commented Jul 9, 2017

It probably means the child has exited before the kill was attempted.

https://nodejs.org/dist/latest-v8.x/docs/api/child_process.html#child_process_child_kill_signal

The ChildProcess object may emit an 'error' event if the signal cannot be delivered.

Contributor

refack commented Jul 9, 2017

It probably means the child has exited before the kill was attempted.

https://nodejs.org/dist/latest-v8.x/docs/api/child_process.html#child_process_child_kill_signal

The ChildProcess object may emit an 'error' event if the signal cannot be delivered.

It's more a case of https://nodejs.org/dist/latest-v8.x/docs/api/process.html#process_process_kill_pid_signal

This method will throw an error if the target pid does not exist.
...
Windows platforms will throw an error if the pid is used to kill a process group.

But presumably in this case we attempt to kill only if we get something from stdout.
I'll try to repro locally. I smell something fishy 👃

Member

richardlau commented Jul 10, 2017

The child process can exit between Node.js reading stdout and calling the on 'data' callback.

jasnell approved these changes

View reviewed changes

addaleax added dont-land-on-v4.x labels

Member

gibfahn commented Jul 12, 2017

@refack can you open a new PR? Now this has been reverted I don't think we should keep using it, it's likely to confuse things like branch-diff (and collaborators!).

IDK if there's a standard way to do this, but don't land a PR twice seems like a reasonable rule.

gibfahn closed this

Contributor

refack commented Jul 12, 2017 •

edited

Loading

@refack can you open a new PR? Now this has been reverted I don't think we should keep using it, it's likely to confuse things like branch-diff (and collaborators!).

IDK if there's a standard way to do this, but don't land a PR twice seems like a reasonable rule.

I consulted with @addaleax and @Trott and this seems like the "standard" way 🤷‍♂️

Member

gibfahn commented Jul 12, 2017

I consulted with @addaleax and @Trott and this seems like the "standard" way 🤷‍♂️

Happy to be wrong if others disagree.

Member

Trott commented Jul 12, 2017

I consulted with @addaleax and @Trott and this seems like the "standard" way 🤷‍♂️

I'm not 100% sure that re-landing from the same PR is the standard way. But if @addaleax says it's OK, that's good enough for me.

Member

addaleax commented Jul 12, 2017

I wouldn’t know that there’s a standard way to handle this either. Opening a new PR is certainly easier for everyone and will make sure the new commit won’t get overlooked, but reverts are probably rare enough that handling them manually is feasible.

Contributor

refack commented Jul 12, 2017

@starkwang Would you like to open a new PR for this?

Contributor Author

starkwang commented Jul 12, 2017

Yes, I'd like to do it. I'm trying to reproduce the flakiness locally.

Contributor

refack commented Jul 12, 2017

@starkwang you could try spawn('type con', { shell: true }) that will start cmd.exe with the right arguments to run type con which is sort of the equivalent to cat with no args

Contributor

refack commented Jul 12, 2017

P.S. @starkwang if you want your full name in the logs you could follow https://help.github.com/articles/setting-your-username-in-git/

Member

gibfahn commented Jul 13, 2017

So at the moment your Git author name and email address are set to:

starkwang <381152119@qq.com>

People usually choose to use their full names for commits. To set your name globally you can do:

git config --global user.name "Weijia Wang"
git config --global user.email "381152119@qq.com"

To change the author for a single commit you can do:

git commit --amend --author="Weijia Wang <381152119@qq.com>"
git push --force-with-lease

It's entirely optional.

Contributor Author

starkwang commented Jul 13, 2017

@gibfahn Thanks for your suggestion! :D
I've changed the user information in Git.

jasnell mentioned this pull request

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

process test windows